Skip to content

Conversation

@sanderegg
Copy link
Member

@sanderegg sanderegg commented Jun 24, 2025

What do these changes do?

Since #7839 (>2 weeks ago), the flow of logs/progresses was broken, as messages sent via SocketIO were not sent through the socketIO RabbitMQ Pub/Sub system but purposely ignoring the queuing since with a sticky connection it was unnecessary.

A second identified problem is that subscription/unsubscription to the project logs/progress RabbitMQ queue needs to be sticky as otherwise the webserver replica will not properly unsubscribe from a topic.

This PR fixes this issue.

Related issue/s

How to test

Dev-ops

@sanderegg sanderegg added this to the Engage milestone Jun 24, 2025
@sanderegg sanderegg requested a review from YuryHrytsuk June 24, 2025 12:45
@sanderegg sanderegg self-assigned this Jun 24, 2025
@sanderegg sanderegg added the a:webserver webserver's codebase. Assigning the area is particularly useful for bugs label Jun 24, 2025
@sanderegg sanderegg requested a review from Copilot June 24, 2025 12:45

This comment was marked as outdated.

@codecov
Copy link

codecov bot commented Jun 24, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.86%. Comparing base (961c173) to head (3620d3f).
Report is 1 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #7967      +/-   ##
==========================================
+ Coverage   87.82%   87.86%   +0.04%     
==========================================
  Files        1849     1842       -7     
  Lines       71343    71153     -190     
  Branches     1250     1250              
==========================================
- Hits        62657    62519     -138     
+ Misses       8324     8272      -52     
  Partials      362      362              
Flag Coverage Δ
integrationtests 64.27% <100.00%> (+<0.01%) ⬆️
unittests 86.46% <100.00%> (+0.03%) ⬆️
Components Coverage Δ
api ∅ <ø> (∅)
pkg_aws_library 93.92% <ø> (ø)
pkg_celery_library 81.13% <ø> (ø)
pkg_dask_task_models_library 79.62% <ø> (ø)
pkg_models_library 93.27% <ø> (ø)
pkg_notifications_library 85.26% <ø> (ø)
pkg_postgres_database 88.18% <ø> (ø)
pkg_service_integration 69.92% <ø> (ø)
pkg_service_library 71.54% <ø> (ø)
pkg_settings_library 90.90% <ø> (ø)
pkg_simcore_sdk 85.05% <ø> (-0.06%) ⬇️
agent 96.29% <ø> (ø)
api_server 92.64% <ø> (ø)
autoscaling 96.03% <ø> (ø)
catalog 92.29% <ø> (ø)
clusters_keeper 99.13% <ø> (ø)
dask_sidecar 91.79% <ø> (ø)
datcore_adapter 97.94% <ø> (ø)
director 76.73% <ø> (-0.10%) ⬇️
director_v2 91.15% <ø> (+0.12%) ⬆️
dynamic_scheduler 96.69% <ø> (ø)
dynamic_sidecar 90.09% <ø> (ø)
efs_guardian 89.65% <ø> (ø)
invitations 93.60% <ø> (ø)
payments 92.57% <ø> (ø)
resource_usage_tracker 88.78% <ø> (-0.06%) ⬇️
storage 86.27% <ø> (ø)
webclient ∅ <ø> (∅)
webserver 87.62% <100.00%> (+<0.01%) ⬆️

Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 961c173...3620d3f. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@sanderegg sanderegg requested a review from Copilot June 24, 2025 13:41
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR restores RabbitMQ-backed delivery for Socket.IO messages by default to prevent loss of logs, progress updates, status changes, and payment events.

  • Default ignore_queue flag is switched to False in the core API and redundant ignore_queue=True calls are removed.
  • Tests and Docker Compose sticky-routing labels are updated to reflect the new default behavior.
  • RabbitMQ consumers and context managers are adjusted to use queued emits.

Reviewed Changes

Copilot reviewed 6 out of 6 changed files in this pull request and generated no comments.

Show a summary per file
File Description
services/web/server/tests/integration/02/notifications/test_rabbitmq_consumers.py Test now expects ignore_queue=False.
services/web/server/src/simcore_service_webserver/socketio/messages.py Updated send_message_to_user signature default and docstring.
services/web/server/src/simcore_service_webserver/projects/_projects_service.py Removed explicit ignore_queue=True in project notifications.
services/web/server/src/simcore_service_webserver/payments/_socketio.py Removed ignore_queue=True for payment notification emits.
services/web/server/src/simcore_service_webserver/notifications/_rabbitmq_exclusive_queue_consumers.py Stripped redundant ignore_queue=True usage and refactored with block syntax.
services/docker-compose.yml Added sticky-routing rules for open/close node operations and set router priority.
Comments suppressed due to low confidence (3)

services/web/server/src/simcore_service_webserver/socketio/messages.py:65

  • [nitpick] The docstring’s default notation {False} may be confusing; consider using a clearer style (e.g., “Defaults to False”) or a standard Sphinx/Google doc param block for consistency.
    ignore_queue: bool = False,

services/web/server/src/simcore_service_webserver/payments/_socketio.py:31

  • With ignore_queue=True removed here, consider adding an integration or unit test to verify that payment completion events now correctly route through RabbitMQ when clients are connected to different server instances.
        ),

services/web/server/src/simcore_service_webserver/projects/_projects_service.py:1961

  • Since ignore_queue=True was removed for project state updates, adding a test to cover cross-instance delivery of project notifications could prevent regressions.
            app,

@sanderegg sanderegg marked this pull request as ready for review June 24, 2025 13:44
@YuryHrytsuk YuryHrytsuk self-requested a review June 24, 2025 14:13
Copy link
Contributor

@YuryHrytsuk YuryHrytsuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the quick fix!

Note based on internal discussion:

  • this PR also fixes priorities for sticky router (they were wrong before)

Copy link
Collaborator

@matusdrobuliak66 matusdrobuliak66 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@sanderegg sanderegg added the 🤖-automerge marks PR as ready to be merged for Mergify label Jun 24, 2025
@sanderegg
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jun 24, 2025

queue

🟠 Waiting for conditions to match

  • any of: [🔀 queue conditions]
    • all of: [📌 queue conditions of queue default]
      • any of: [🛡 GitHub branch protection]
        • check-neutral = integration-tests
        • check-skipped = integration-tests
        • check-success = integration-tests
      • #approved-reviews-by >= 2 [🛡 GitHub branch protection]
      • #approved-reviews-by>=2
      • #changes-requested-reviews-by = 0 [🛡 GitHub branch protection]
      • #changes-requested-reviews-by=0
      • #review-threads-unresolved = 0 [🛡 GitHub branch protection]
      • #review-threads-unresolved=0
      • -conflict
      • -draft
      • base=master
      • branch-protection-review-decision = APPROVED [🛡 GitHub branch protection]
      • label!=🤖-do-not-merge
      • label=🤖-automerge
      • any of: [🛡 GitHub branch protection]
        • check-skipped = deploy to dockerhub
        • check-neutral = deploy to dockerhub
        • check-success = deploy to dockerhub
      • any of: [🛡 GitHub branch protection]
        • check-success = system-tests
        • check-neutral = system-tests
        • check-skipped = system-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = unit-tests
        • check-neutral = unit-tests
        • check-skipped = unit-tests
      • any of: [🛡 GitHub branch protection]
        • check-success = check OAS' are up to date
        • check-neutral = check OAS' are up to date
        • check-skipped = check OAS' are up to date
      • any of: [🛡 GitHub branch protection]
        • check-success = build-test-images (frontend) / build-test-images
        • check-neutral = build-test-images (frontend) / build-test-images
        • check-skipped = build-test-images (frontend) / build-test-images
  • -closed [📌 queue requirement]
  • -conflict [📌 queue requirement]
  • -draft [📌 queue requirement]
  • any of: [📌 queue -> configuration change requirements]
    • -mergify-configuration-changed
    • check-success = Configuration changed

@sanderegg sanderegg force-pushed the bugfix/socketio-messages-sent-to-user-lost branch 2 times, most recently from 0a6268d to 2cca3b1 Compare June 24, 2025 15:02
@sanderegg sanderegg force-pushed the bugfix/socketio-messages-sent-to-user-lost branch from 2cca3b1 to 3620d3f Compare June 24, 2025 16:06
@sanderegg
Copy link
Member Author

@mergify queue

@mergify
Copy link
Contributor

mergify bot commented Jun 24, 2025

queue

✅ The pull request has been merged automatically

The pull request has been merged automatically at 2d1134f

@sonarqubecloud
Copy link

@mergify mergify bot merged commit 2d1134f into ITISFoundation:master Jun 24, 2025
150 of 152 checks passed
@sanderegg sanderegg deleted the bugfix/socketio-messages-sent-to-user-lost branch June 24, 2025 17:07
@giancarloromeo giancarloromeo mentioned this pull request Jun 26, 2025
1 task
matusdrobuliak66 pushed a commit that referenced this pull request Jun 26, 2025
@pcrespov pcrespov mentioned this pull request Jul 2, 2025
@matusdrobuliak66 matusdrobuliak66 mentioned this pull request Aug 5, 2025
88 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🤖-automerge marks PR as ready to be merged for Mergify a:webserver webserver's codebase. Assigning the area is particularly useful for bugs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

logs stop flowing with current sticky configuration

4 participants